Table Scan Delete File Handling: Positional and Equality Delete Support #652

sdd · 2024-09-27T07:44:31Z

This PR adds support for handling of both positional and equality delete files within table scans.

The approach taken is to include a list of delete file paths in every FileScanTask. At the moment it is assumed that this list refers to delete files that may apply to the data file in the scan, rather than having been filtered to only contain delete files that definitely do apply to this data file. Further optimisation of plan_files is expected in the future to ensure that this list is pre-filtered before being included in the FileScanTask.

This PR has been refactored so that this work can be split into multiple PRs.

This PR now contains solely the scan plan changes.

sdd · 2024-10-10T18:27:55Z

@Xuanwo, @liurenjie1024: This is now ready to review, PTAL when you guys get chance. Look forward to your feedback 😁

sdd · 2024-10-29T08:30:51Z

Hi @liurenjie1024 and @Xuanwo - would either of you be able to review this at some point please? I know it's a bit large, sorry. Thanks :-)

liurenjie1024 · 2024-10-29T08:33:48Z

Hi @liurenjie1024 and @Xuanwo - would either of you be able to review this at some point please? I know it's a bit large, sorry. Thanks :-)

Hi, @sdd Thanks for your patience. In fact I already started reviewing it, and it's a little large, so it may take some time.

sdd · 2024-10-31T08:28:39Z

Hey @liurenjie1024 - sorry to make changes whilst you are reviewing. I updated the design of the DeleteFileManager as I was not happy with it.

liurenjie1024

Thanks @sdd for your patience, it's a really large pr and took me some time to review. I think generally you've understood how deletion files works, but I have some concerns about current code as it mixed a lot of things together. Deletion hanndling is quite chanllenging, I think the design from java implemention is quite reasonable:

I think maybe we need to have a design to split them into more small parts, what do you think?

crates/iceberg/src/spec/delete_file.rs

crates/iceberg/src/scan.rs

liurenjie1024 · 2024-11-04T12:48:27Z

crates/iceberg/src/scan.rs

+
+/// A task to scan part of file.
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
+pub struct FileScanTaskDeleteFile {


This may evolve as we add more feature, so I would suggest to make this a crate only data structure.

Are you sure? It is present inside FileScanTask, all of whose items are pub and is intended for potential consumption outside of the crate.

crates/iceberg/src/spec/delete_file.rs

crates/iceberg/src/arrow/reader.rs

liurenjie1024 · 2024-11-04T13:19:33Z

crates/iceberg/src/scan.rs

+        //       that are not applicable to the DataFile?
+
+        DeleteFileManagerFuture {
+            files: self.files.clone(),


This is incorrect even if we ignore pruning techniques to remove unrelated deletion files. Please see this part for details.

sdd · 2024-11-12T10:14:51Z

Thanks so much for the review on this @liurenjie1024 - I've been ill for the past week or two so I've not had chance to work through your review in detail yet. I just wanted to let you know I've seen it and will pick it up when I've recovered. 👍

liurenjie1024 · 2024-11-13T03:01:53Z

Thanks so much for the review on this @liurenjie1024 - I've been ill for the past week or two so I've not had chance to work through your review in detail yet. I just wanted to let you know I've seen it and will pick it up when I've recovered. 👍

Hi, @sdd Sorry to hear that, take care of yourself! Don't worry about this, I'll be happy to discuss about this with you anytime when you're back.

Fokko · 2024-12-19T17:13:39Z

@sdd Thanks for doing all this work, could you split out the positional deletes? I think that's already a sizeable chunk.

sdd · 2024-12-19T18:12:14Z

Sure @Fokko - I'm in the middle of a refactor of what I have so far. It aligns the design a bit more closely to the Java DeleteFileIndex while still keeping the more efficient loading process from my original. I was thinking of splitting this PR into three - one that is mostly collating all the delete files into the index, and then two more that each focus on the filtering and application of the two delete types.

crates/iceberg/src/delete_file_index.rs

Fokko · 2024-12-20T12:13:34Z

@sdd Thank you for your understanding, looking forward to the smaller PRs 👍 From PyIceberg I've learned that there are a lot of subtle optimizations and want to make sure that we handle those correctly 👍

sdd · 2024-12-23T17:43:15Z

FAO @liurenjie1024, @Xuanwo, @Fokko:

I've finished refactoring this and after a few rounds I'm happier with the design of the DeleteFileIndex and how it is interacted with in the scan plan phase.

I will follow up once this is merged with another PR similar to @Xuanwo's recent one as I think we can use similar techniques in the scan plan.

I've got a few TODOs in here that mark behaviour that I was unsure of and could do with feedback upon, as well as to indicate missing parts that will be addressed in follow-up PRs.

Used to encapsulate enough information for DeleteFileIndex to be able to perform the requrired filtering of delete files

…ntime::spawn. Add missing license

…deletes for data file.

…t in a file scan task

sdd mentioned this pull request Sep 27, 2024

Delete Files in Table Scans #630

Open

sdd force-pushed the feature/table-scan-delete-file-handling branch 3 times, most recently from 0a64237 to 28021a4 Compare October 10, 2024 18:03

sdd marked this pull request as ready for review October 10, 2024 18:26

sdd changed the title ~~WIP: Table Scan Delete File Handling~~ Table Scan Delete File Handling: Positional Delete Support Oct 10, 2024

sdd force-pushed the feature/table-scan-delete-file-handling branch from 2732a49 to 50f8a9e Compare October 24, 2024 17:46

sdd changed the title ~~Table Scan Delete File Handling: Positional Delete Support~~ Table Scan Delete File Handling: Positional and Equality Delete Support Oct 24, 2024

sdd force-pushed the feature/table-scan-delete-file-handling branch 3 times, most recently from cf8748a to 7a8d297 Compare October 28, 2024 07:28

sdd force-pushed the feature/table-scan-delete-file-handling branch from cc5dba4 to df4e86a Compare October 31, 2024 08:23

liurenjie1024 reviewed Nov 4, 2024

View reviewed changes

c-thiel mentioned this pull request Nov 16, 2024

Tracking issue: Writing iceberg tables #346

Closed

7 tasks

Fokko self-requested a review November 28, 2024 15:24

sdd force-pushed the feature/table-scan-delete-file-handling branch 2 times, most recently from 2ff526f to 091a249 Compare December 11, 2024 19:07

Xuanwo reviewed Dec 20, 2024

View reviewed changes

crates/iceberg/src/delete_file_index.rs Show resolved Hide resolved

Fokko mentioned this pull request Dec 20, 2024

feat: exposing delete files in task #625

Closed

sdd force-pushed the feature/table-scan-delete-file-handling branch 2 times, most recently from 9e9ee48 to a7493f0 Compare December 23, 2024 11:49

sdd added 10 commits December 24, 2024 08:34

feat: add delete_file_index and populate in table scan

5a43fc8

refactor: introduce DeleteFileContext.

f4f841e

Used to encapsulate enough information for DeleteFileIndex to be able to perform the requrired filtering of delete files

chore: remove unused BoundPredicate::and method and unused import

53fbc29

refactor: get_deletes_for_data_file. Change tokio::spawn to crate::ru…

7c1bd09

…ntime::spawn. Add missing license

refactor: DeleteFileIndex cloneable, with an awaitable method to get …

8cfc923

…deletes for data file.

style: reorder

ad52669

style: remove unneeded commented-out code

c24db29

refactor: collect delete file results into vec

274cc55

fix: ensure arrow reader returns Unsupported when delete files presen…

a3b34b2

…t in a file scan task

feat: global delete support, DeleteFileIndex

22ef190

sdd force-pushed the feature/table-scan-delete-file-handling branch from d3e4d0d to 22ef190 Compare December 24, 2024 08:36

style: reformat with cargo fmt

aae3896

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table Scan Delete File Handling: Positional and Equality Delete Support #652

Table Scan Delete File Handling: Positional and Equality Delete Support #652

sdd commented Sep 27, 2024 •

edited

Loading

sdd commented Oct 10, 2024

sdd commented Oct 29, 2024

liurenjie1024 commented Oct 29, 2024

sdd commented Oct 31, 2024

liurenjie1024 left a comment

liurenjie1024 Nov 4, 2024

sdd Dec 9, 2024

liurenjie1024 Nov 4, 2024

sdd commented Nov 12, 2024

liurenjie1024 commented Nov 13, 2024

Fokko commented Dec 19, 2024

sdd commented Dec 19, 2024

Fokko commented Dec 20, 2024

sdd commented Dec 23, 2024

Table Scan Delete File Handling: Positional and Equality Delete Support #652

Are you sure you want to change the base?

Table Scan Delete File Handling: Positional and Equality Delete Support #652

Conversation

sdd commented Sep 27, 2024 • edited Loading

sdd commented Oct 10, 2024

sdd commented Oct 29, 2024

liurenjie1024 commented Oct 29, 2024

sdd commented Oct 31, 2024

liurenjie1024 left a comment

Choose a reason for hiding this comment

liurenjie1024 Nov 4, 2024

Choose a reason for hiding this comment

sdd Dec 9, 2024

Choose a reason for hiding this comment

liurenjie1024 Nov 4, 2024

Choose a reason for hiding this comment

sdd commented Nov 12, 2024

liurenjie1024 commented Nov 13, 2024

Fokko commented Dec 19, 2024

sdd commented Dec 19, 2024

Fokko commented Dec 20, 2024

sdd commented Dec 23, 2024

sdd commented Sep 27, 2024 •

edited

Loading